Callback from a notification handler can deadlock #78
Conversation
|
Ah, interesting. Yes, I see the trouble: The replies from the callback are handled during the ordinary request processing sequence. I think I know how to fix this, but I'll have to test out a couple of things. Thanks for reporting the issue, and for the helpful reproduction case. |
|
The root problem is that the barrier we added in #21, to address the concern raised in #20, prevents any further request batches from being processed. Roughly, the sequence of events is this:
At this point, however, the server will not remove any further items from the queue, by design: We can't issue any further handlers until the barrier completes, to preserve order. In this case, however, we still want to process messages that do not implicate handlers, i.e,. the response from a callback. I believe we can do this by front-loading the handling of responses. That should be safe to do disregarding the barrier. |
Previously, a notification handler that issues a call back to the client could block delivery of the reply for its own callback: The barrier we use to preserve issue order means another batch cannot be issued to the dispatcher until all previously-issued notifications have completed. To prevent the handler from deadlocking itself in this case, filter out response messages from the client when the input is received, rather than enqueuing them with the handlers. This basically just moves the existing logic earlier in the transaction, but it means replies can be delivered even if the barrier is active. Fixes #78.
|
Can you please try patching #79 to see if it addresses your issue? It fixes the repro in the test case, but I assume that is a palimpsest of a more interesting usage. |
Previously, a notification handler that issues a call back to the client could block delivery of the reply for its own callback: The barrier we use to preserve issue order means another batch cannot be issued to the dispatcher until all previously-issued notifications have completed. To prevent the handler from deadlocking itself in this case, filter out response messages from the client when the input is received, rather than enqueuing them with the handlers. This basically just moves the existing logic earlier in the transaction, but it means replies can be delivered even if the barrier is active. Fixes #78. * Add regression test for deadlock bug. Co-authored-by: Radek Simko <radek.simko@gmail.com>
I ran into a bug which appears to be very similar to #27
As demonstrated by the attached (failing) test:
If there's any
client -> servernotification in-flight while a server notification handler is waiting for client to respond to its request, theCallback()method effectively hangs (even though the client has clearly received the request) and no further requests are processed, effectively causing a deadlock.One particular (IMO valid) use case where this came up is when I tried to implement LSP server-initiated progress in terraform-ls. That means calling
window/workDoneProgress/createfrom eitherinitializedortextDocument/didOpennotification handlers.